Improvements to payload agent - #511

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@plugins/ci/commands/payload-analysis.md`:
- Around line 12-14: The Markdown file plugins/ci/commands/payload-analysis.md
has fenced code blocks missing language identifiers (tripping MD040); update the
fences so the usage block uses a neutral language like "text" (for the usage
line `/ci:payload-analysis <payload-tag> [--snapshot-dir DIR]`) and the example
invocation blocks use "bash" (for the example commands such as
`/ci:payload-analysis 4.22.0-0.nightly-2026-02-25-152806`, `/ci:payload-analysis
... --snapshot-dir payload/4.22/nightly`, and the arm64 example); apply the same
change to the other affected fences around lines 76-88 so all triple-backtick
fences include the appropriate language identifiers.

In `@plugins/ci/evals/scripts/extract-payload-analysis-snapshots.sh`:
- Line 33: The tar extraction command tar xzf "$archive" -C "$dest" is
vulnerable to path traversal and link attacks; before extracting, enumerate
members (tar -tzf "$archive") and validate each entry: reject any member with an
absolute path (starts with '/'), any path containing '..' segments, and any
member that is a symlink or hardlink (detect via tar -tzvf or --list with type
flag and reject type 'l'/'L'/'H' entries); only if all members pass, proceed to
extract to "$dest" (tar xzf "$archive" -C "$dest"), otherwise abort with a clear
error.

In `@plugins/ci/skills/payload-analysis/SKILL.md`:
- Around line 137-147: The fenced code blocks in SKILL.md (notably the block
labeled "ANALYSIS_RESULT" and the other plain-output blocks referenced around
the sections showing job summaries) are missing language identifiers which trips
markdown lint; update each ``` fence to include a language token (use ```text
for plain output) so all fenced blocks like the ANALYSIS_RESULT block and the
other plain-output blocks are written as ```text ... ``` to satisfy the linter.
- Around line 1-4: The SKILL.md for payload-analysis is missing the required
4-phase ai-helpers overlap analysis; update the payload-analysis SKILL.md to
include a 4-phase overlap report (Phase 1–4) that classifies similarity as
HIGH/MODERATE/LOW against existing plugins/ci/* payload components and any
overlapping open PRs, include explicit lists of overlapping component names and
PR identifiers/links, state the methodology and metrics used for similarity
scoring, and summarize actionable conclusions (e.g., merge/reuse/avoid,
potential conflicts) so reviewers can see which components/PRs overlap and why.

In `@plugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py`:
- Line 22: The JUnit XML parsing in _parse_junit_xml currently uses
xml.etree.ElementTree (imported as ET) which is vulnerable to XML bomb/entity
expansion; replace it by adding defusedxml as a dependency, change the import at
the top to use defusedxml's ElementTree (e.g., import defusedxml.ElementTree as
ET or from defusedxml import ElementTree as ET), keep using ET.parse(source)
inside _parse_junit_xml, and update the exception handling around the parse to
also catch defusedxml's security exception
(defusedxml.common.DefusedXmlException) in addition to ET.ParseError and OSError
so malicious payloads are rejected safely and logged.

In `@plugins/ci/skills/payload-snapshot/SKILL.md`:
- Around line 120-123: The jq example incorrectly reads component from each
commit's .name; update the query to take the component name from the parent
updatedImages[] object by binding the parent (e.g., using updatedImages[] as
$img) and then iterating $img.commits[] and using $img.name for the component
while keeping .pullURL and .subject for pr and subject; change the expression
that currently uses .changeLogJson.updatedImages[].commits[] and .name to
reference the parent’s name (e.g., $img.name) instead.

---

Nitpick comments:
In `@plugins/ci/commands/payload-revert.md`:
- Around line 39-42: Add a language specifier to the fenced code block
containing the error message "Error: Payload results YAML not found for
{payload_tag}." so it becomes a code fence like ```text; update the opening
backticks for the block that includes the "Error: Payload results YAML not found
for {payload_tag}." and the "Run `/ci:payload-analysis {payload_tag}` first to
generate it." line to satisfy markdownlint and improve consistency.

In
`@plugins/ci/evals/cases/payload-analysis/case-012-4.20-rejected-streak/annotations.yaml`:
- Around line 1-12: Add an explicit expected_candidates field to the annotations
so the file consistently declares expected revert candidates; since
has_revert_candidates is false, set expected_candidates to an empty array
(expected_candidates: []). Update the top-level keys (e.g., alongside
expected_phase, expected_failed_job_count, has_revert_candidates) so consumers
that expect the expected_candidates key won't break and test case formatting
matches case-011.

In
`@plugins/ci/evals/cases/payload-analysis/case-013-4.20-accepted-with-failures/annotations.yaml`:
- Around line 1-11: Add an explicit expected_candidates field to the annotations
YAML (e.g., expected_candidates: []) to mirror case-011 and ensure consistency
with has_revert_candidates: false; update the top-level fields in the same
document where expected_phase, expected_failed_job_count, and
has_revert_candidates are declared so the evaluation framework sees an empty
array for expected_candidates rather than omitting the key.

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@plugins/ci/skills/payload-autodl-json/scripts/validate.py`:
- Around line 42-50: The loop over rows is both prematurely breaking (the "if i
> 0: break" stops after the first row) and assumes each row is a dict (using
row.items()), which can crash on malformed input; remove the premature break so
all rows are validated, and before accessing row.items() or checking fields
against REQUIRED_ROW_FIELDS ensure row is a dict (or mapping) — if not, append
an error like "rows[i] is not an object" and continue; only then check missing
fields and non-string values (use the existing REQUIRED_ROW_FIELDS, rows
variable and the enumerate loop) so every row is safely validated without
exceptions.

In `@plugins/ci/skills/payload-results-yaml/scripts/validate.py`:
- Around line 39-43: The loops over data["failing_jobs"] and data["candidates"]
assume each item is a mapping and will crash on non-dict entries; update the
checks in the failing_jobs loop (iterate variable job in validate.py) and the
candidates loop to first verify isinstance(item, dict) and if not append a clear
error like "failing_jobs[i] is not an object" or "candidates[j] is not an
object" then continue, otherwise perform the existing REQUIRED_JOB_FIELDS /
REQUIRED_CANDIDATE_FIELDS membership checks as before.
- Around line 29-33: The validation currently assumes metadata is a dict by
doing meta = data.get("metadata", {}) and then iterating REQUIRED_METADATA,
which allows non-object metadata (e.g., string or list) to pass or produce
confusing messages; update the logic where meta is read (the variable meta
derived from data.get("metadata", {})) to first check that metadata exists and
is a mapping/dict type and, if not, append a clear error like "metadata must be
an object" to errors and skip the REQUIRED_METADATA loop; keep using the same
symbols (data, meta, REQUIRED_METADATA, errors) so callers and tests still find
the check location.

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@plugins/ci/skills/payload-autodl-json/scripts/test_validate.py`:
- Around line 36-38: The temporary file created with
tempfile.NamedTemporaryFile(..., delete=False) in test_validate.py is never
removed; update the test to ensure the file is cleaned up after use by removing
the file (os.unlink or pathlib.Path.unlink) once the test case is registered or
in test teardown, or change to delete=True and capture the name before closing
if necessary; locate the NamedTemporaryFile usage and add explicit cleanup
(e.g., try/finally or fixture teardown) to avoid leaving orphaned files.

In `@plugins/ci/skills/payload-results-yaml/scripts/test_validate.py`:
- Around line 38-40: The temp file created with tempfile.NamedTemporaryFile(...,
delete=False) in the test is never removed; update the test in test_validate.py
so the temporary YAML is deleted after use (either use delete=True or explicitly
unlink the file), e.g. create the temp file, append the case to cases, then
ensure os.unlink(f.name) in a finally block or use pytest's tmp_path fixture to
manage cleanup; refer to the NamedTemporaryFile call and the variable f and the
cases list when making the change.